Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvements around empty data handling #18

Merged
merged 7 commits into from
Feb 15, 2019
Merged

Improvements around empty data handling #18

merged 7 commits into from
Feb 15, 2019

Conversation

kurtisnelson
Copy link
Contributor

No description provided.

Copy link

@ZacSweers ZacSweers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First CR on this - why is SimpleStore closeable? Seems strange that the entire store is closeable vs having some sort of transaction that is instead. What if multiple spots access it? Or is SimpleStore the transaction and consumers must always create a new instance from the factory?

@@ -0,0 +1,10 @@
syntax = "proto2";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not proto3?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

proto3 doesn't buy anything on mobile and in fact has downsides.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a bit vague - can you elaborate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -28,6 +29,7 @@
private static final int OPEN = 0;
private static final int CLOSED = 1;
private static final int TOMBSTONED = 2;
private static final byte[] EMPTY_BYTES = new byte[0];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this preferable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is cleaner than constantly allocating an empty byte array.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not what I meant - why is an empty array preferable over null?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Futures should not have nulls passed through them just like Observables should not. It is up to the user/data model chose to represent optionality in the model itself instead of the framework requiring everyone everywhere to handle null.

@kurtisnelson
Copy link
Contributor Author

First CR on this - why is SimpleStore closeable? Seems strange that the entire store is closeable vs having some sort of transaction that is instead. What if multiple spots access it? Or is SimpleStore the transaction and consumers must always create a new instance from the factory?

SimpleStore is a way of accessing a specific scope and guaranteeing order within that scope.

Verified

This commit was signed with the committer’s verified signature.
kurtisnelson Kurt Nelson
* Support cache scope config
* Handle empty state properly

Verified

This commit was signed with the committer’s verified signature.
kurtisnelson Kurt Nelson

Verified

This commit was signed with the committer’s verified signature.
kurtisnelson Kurt Nelson

Verified

This commit was signed with the committer’s verified signature.
kurtisnelson Kurt Nelson

Verified

This commit was signed with the committer’s verified signature.
kurtisnelson Kurt Nelson
Never return null from store methods.

Verified

This commit was signed with the committer’s verified signature.
kurtisnelson Kurt Nelson

Verified

This commit was signed with the committer’s verified signature.
kurtisnelson Kurt Nelson
protosimplestore/build.gradle Show resolved Hide resolved
@@ -55,6 +73,14 @@ public static SimpleProtoStoreImpl create(Context context, String scope, ScopeCo
SimpleStoreConfig.getComputationExecutor());
}

@Override
public ListenableFuture<Boolean> contains(String key) {
return Futures.transform(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could consider an index here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will optimize contains down the road.

return create(context, scope, ScopeConfig.DEFAULT);
}

public static SimpleProtoStore create(Context context, String scope, ScopeConfig config) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

follow up on discussion if a wrap is appropriate

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

following up with some prototyping later

@kurtisnelson kurtisnelson merged commit f95b18e into master Feb 15, 2019
@kurtisnelson kurtisnelson deleted the tests branch February 15, 2019 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants